[Fixes #14552] Fragile /api/v2/ router URL registration - #14559
Open
etj wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a fragile /api/v2/ URL registration issue caused by router.urls being a cached property and some endpoints being registered/injected too late (via AppConfig.ready()), making routes intermittently missing depending on app-loading order.
Changes:
- Reorders and groups
/api/v2/URL includes so allrouter.register()side effects occur before the single canonicalinclude(router.urls). - Removes the dead upload URL override/injection mechanism and related empty URLconf.
- Hoists common URL prefixes (metadata, facets, management commands) into
geonode/urls.pyand adds regression tests to guard against router completeness regressions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| geonode/urls.py | Groups /api/v2/ registrations and moves include(router.urls) to the end to avoid cached/partial router URL sets. |
| geonode/upload/apps.py | Removes dead ready() hook that attempted to inject upload API URLs. |
| geonode/upload/api/urls.py | Deletes unused/empty upload API URLconf. |
| geonode/resource/apps.py | Removes URL injection from AppConfig.ready() (registration now handled via URL includes). |
| geonode/metadata/api/urls.py | Stops reading router.urls inside the module; keeps only metadata extra endpoints and relies on the global router include. |
| geonode/management_commands_http/urls.py | Drops internal management/ prefix to rely on the include prefix from geonode/urls.py. |
| geonode/harvesting/apps.py | Removes URL injection from AppConfig.ready(); keeps signal wiring and scheduler setup. |
| geonode/facets/urls.py | Updates facets URL patterns to rely on the include prefix from geonode/urls.py. |
| geonode/api/tests.py | Adds regression tests ensuring the shared router remains complete and no longer depends on ready() URL injection. |
Suppressed comments (1)
geonode/facets/urls.py:26
path()routes cannot start with a leading/in Django; this will raiseImproperlyConfiguredwhen loading the URLconf. With the current include prefix (^api/v2/facets), the remaining path for the detail view begins with/, so this should be expressed as are_path()regex instead.
path("/<facet>", GetFacetView.as_view(), name="get_facet"),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -20,7 +20,8 @@ | |||
| from django.urls import path | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Quick list:
urlpatterns +=cosmetic merges^api/v2/registrations groupedPR description:
Fixes the
/api/v2/router registration bug:router.urlsis a cached property, andresource/harvestingused to inject their URLs fromAppConfig.ready()-- which runs aftergeonode/urls.py's own code -- so anything registered only that way could be silently missing depending on app-loading order. They're hardcoded includes now, like every other app, and the canonicalinclude(router.urls)is positioned after everything that can register onto it.Also, while in the area:
geonode-importerwas vendored in.urlpatterns +=splits with no functional reason to be separate, and groups the/api/v2/registrations into one block.metadata/facets/managementprefixes out of those apps' ownurls.pyinto theinclude()ingeonode/urls.py, removing the repeated prefix in every one of their paths.No behavior change beyond the fix itself -- every step verified by diffing the fully resolved URLconf before/after (with
PYTHONHASHSEEDpinned, since DREST's field-name route ordering is otherwise non-deterministic per process).Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.